-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix]
Part-1 - LevelDbStore
#3414
base: master
Are you sure you want to change the base?
[Fix]
Part-1 - LevelDbStore
#3414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this PR be tested without PART 2?
Yes |
Hey Chris, can you please add a few UTs to demonstrate how your pr improves the leveldb? I am reviewing your code, but dont really clear how and where the improve works. |
|
Will check part 2 as well then, but please avoid doing this, cause it makes a single pr not reviewable. |
Just trying to make it small. |
Added |
@superboyiii can you please rake a look at this pr and do some tests on it? |
Sure |
@cschuchardt88 Seems not an issue from this PR because it occurs on master in block 2724677, I will open an issue. |
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Shargon <[email protected]>
if (disposing) | ||
{ | ||
FreeManagedObjects(); | ||
} | ||
if (Handle != nint.Zero) | ||
{ | ||
FreeUnManagedObjects(); | ||
Handle = nint.Zero; | ||
} | ||
_disposed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to use Interlocked
for thread safe flag use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable I'll add it in when I get a chance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you can't Interlocked
. its a non-ref returning type. Reason for locking it?
@superboyiii Require testing in x86 and x64 machines |
@shargon I added the |
OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retsted with Part2, it works well and stable. No data imcompatible. I don't have a way to get a x86 but it works well on x64.
@shargon any input would be nice. so we can get this going. |
return db.Get(options, key); | ||
} | ||
public byte[] TryGet(byte[] key) => | ||
_db.Get(key, _readOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we use lock to write we also need it to read, or you could be reading while someone else is writing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why you take/use a snapshot.
Native.leveldb_close(handle); | ||
handle = IntPtr.Zero; | ||
} | ||
Put(key, value, new WriteOptions()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache write options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Description
This fixes the store's
dotnet
memory management forGC
, a long with adding new feature options, easierException
catching.Also exposes the store.
Change Log
ReadOptions
,WriteOptions
andOptions
.LevelDBHandle.cs
forGC
and easy management.IEnumerable<KeyValuePair<byte[], byte[]>>
toStore.cs
toIterate
over the whole store.namespace
fromNeo.IO.Data.LevelDB
toNeo.IO.Storage.LevelDB
MaxOpenFiles=4096
,SnappyCompression
andFilterPolicy=10
option toStore
.thread-safety
tobatching
sinceleveldb
doesn't supportmulti-thread
in batching.Type of change
How Has This Been Tested?
Unit tests
Checklist: